Conversation
c7be8dd to
90b4eee
Compare
|
One of the public type files has been updated, plase make sure there are no backwards incompatible changes done in the PR. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #5922 +/- ##
==========================================
+ Coverage 93.73% 93.94% +0.21%
==========================================
Files 641 647 +6
Lines 138167 140265 +2098
Branches 14561 14705 +144
==========================================
+ Hits 129510 131777 +2267
+ Misses 8657 8488 -169
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
One of the public type files has been updated, plase make sure there are no backwards incompatible changes done in the PR. |
|
One of the public type files has been updated, plase make sure there are no backwards incompatible changes done in the PR. |
1 similar comment
|
One of the public type files has been updated, plase make sure there are no backwards incompatible changes done in the PR. |
|
One of the public type files has been updated, plase make sure there are no backwards incompatible changes done in the PR. |
… calculations # Conflicts: # src/virtual_renderer.js
|
One of the public type files has been updated, plase make sure there are no backwards incompatible changes done in the PR. |
…` and related methods
…ed text-to-pixel accuracy and update related cursor positioning logic fix: remove unused `offsetX` parameter from `screenToDocumentPosition` and related methods fix: set $fontMetrics to session instead of adding new selection methods fix: refine cursor positioning using pixel-based calculations # Conflicts: # src/layer/font_metrics.js # src/selection.js
|
One of the public type files has been updated, plase make sure there are no backwards incompatible changes done in the PR. |
|
One of the public type files has been updated, plase make sure there are no backwards incompatible changes done in the PR. |
|
One of the public type files has been updated, plase make sure there are no backwards incompatible changes done in the PR. |
|
One of the public type files has been updated, plase make sure there are no backwards incompatible changes done in the PR. |
|
One of the public type files has been updated, plase make sure there are no backwards incompatible changes done in the PR. |
|
One of the public type files has been updated, plase make sure there are no backwards incompatible changes done in the PR. |
|
One of the public type files has been updated, plase make sure there are no backwards incompatible changes done in the PR. |
|
One of the public type files has been updated, plase make sure there are no backwards incompatible changes done in the PR. |
|
One of the public type files has been updated, plase make sure there are no backwards incompatible changes done in the PR. |
There was a problem hiding this comment.
Nice work, this is awesome!
I built #5988 on an earlier snapshot of this branch, so below are a few things I hit while testing grapheme/bidi edge cases against real browser measurements. Most are small and cherry-pickable from #5988. Details inline.
Once this merges I'd like us to review and maybe do a follow-up PR picking up the rest of #5988: the grapheme-cluster model (Intl.Segmenter-based helpers in lib/lang, cluster-snapping moveCursorTo, cluster-aware $getDisplayTokens/$computeWrapSplits, $alignTokensToClusters). It touches the same code this PR rewrites, so rebasing it on top of merged master is cleaner than growing this PR's scope. With the current branch a wrap limit crossing 👨👩👧👦 still splits its 11 code units across screen rows, and moveCursorTo only snaps surrogate pairs, not ZWJ/combining-mark clusters; the follow-up addresses #460, #4142, #5431, #4602, #3753, #3866, #3617.
| $renderToken(parent, screenColumn, token, value) { | ||
| var self = this; | ||
| var re = /(\t)|( +)|([\x00-\x1f\x80-\xa0\xad\u1680\u180E\u2000-\u200f\u2028\u2029\u202F\u205F\uFEFF\uFFF9-\uFFFC\u2066\u2067\u2068\u202A\u202B\u202D\u202E\u202C\u2069\u2060\u2061\u2062\u2063\u2064\u206A\u206B\u206B\u206C\u206D\u206E\u206F]+)|(\u3000)|([\u1100-\u115F\u11A3-\u11A7\u11FA-\u11FF\u2329-\u232A\u2E80-\u2E99\u2E9B-\u2EF3\u2F00-\u2FD5\u2FF0-\u2FFB\u3001-\u303E\u3041-\u3096\u3099-\u30FF\u3105-\u312D\u3131-\u318E\u3190-\u31BA\u31C0-\u31E3\u31F0-\u321E\u3220-\u3247\u3250-\u32FE\u3300-\u4DBF\u4E00-\uA48C\uA490-\uA4C6\uA960-\uA97C\uAC00-\uD7A3\uD7B0-\uD7C6\uD7CB-\uD7FB\uF900-\uFAFF\uFE10-\uFE19\uFE30-\uFE52\uFE54-\uFE66\uFE68-\uFE6B\uFF01-\uFF60\uFFE0-\uFFE6]|[\uD800-\uDBFF][\uDC00-\uDFFF])/g; | ||
| var re = /(\t)|( +)|([\x00-\x1f\x80-\xa0\xad\u1680\u180E\u2000-\u200f\u2028\u2029\u202F\u205F\uFEFF\uFFF9-\uFFFC\u2066\u2067\u2068\u202A\u202B\u202D\u202E\u202C\u2069\u2060\u2061\u2062\u2063\u2064\u206A\u206B\u206B\u206C\u206D\u206E\u206F]+)|(\u3000+)/g; |
There was a problem hiding this comment.
This control-character class still includes U+200D (ZWJ) via the \u2000-\u200f range, so joined emoji like 👨👩👧👦 render chopped up by invalid-char dots. Splitting the range to \u2000-\u200C\u200E\u200f keeps ZWJ sequences intact and fixes #5813. (This is the one-character version of what I did in #5988.)
| var rects = getRects(child); | ||
| for (var j = 0; j < rects.length; j++) { | ||
| let rect = rects[j]; | ||
| if (rect.left < x && x < rect.left + rect.width) { |
There was a problem hiding this comment.
Strict inequalities here mean a click landing exactly on the seam between two spans (e.g. with showInvisibles on) matches neither child rect, falls through, and accumulates the wrong column. rect.left <= x && x <= rect.left + rect.width fixes it. Also worth clamping to column 0 when x is left of the line start; right now that path can return a large column.
| maxDistance = d; | ||
| } | ||
| } | ||
| if (blockCursor) { |
There was a problem hiding this comment.
if (blockCursor) { // TODO } is a no-op, so vim block-cursor mode gets round-to-nearest instead of floor semantics. Either implement or drop the branch and note the limitation.
|
|
||
| var cursorTop = (pos.row - (onScreen ? this.config.firstRowScreen : 0)) * | ||
| this.config.lineHeight; | ||
| var cursorWidth = this.config.fontMetrics.textWidth(pos.row, pos.column + 1) - textWidth; |
There was a problem hiding this comment.
If textWidth(col+1) and textWidth(col) measure equal (zero-width cluster tail, or a measurement fallback), cursorWidth is 0 and the cursor becomes invisible. Suggest (... - textWidth) || this.config.characterWidth.
| this.updateBidiMap(); | ||
| } | ||
| return this.bidiMap.bidiLevels; | ||
| return true; |
There was a problem hiding this comment.
isBidiRow now returns true for every row once the document contains any bidi character. One Hebrew word in a 10k-line file routes every marker on every line through drawBidiSingleLineMarker → getRects → DOM range measurement. A cheap per-line bidiRE.test(line) here would keep the fast path for the 99% of lines that aren't bidi.
| if (useFontMetrics && this.$desiredColumn == null) { | ||
| offsetX = fontMetrics.textWidth(screenPos.row, screenPos.column); | ||
| if (isFinite(offsetX)) | ||
| this.$desiredColumn = offsetX / fontMetrics.config.characterWidth; |
There was a problem hiding this comment.
$desiredColumn now stores a fractional pixel-derived value (offsetX / characterWidth). When the target row isn't rendered, $pixelToColumn returns it unchanged and a non-integer column flows into screenToDocumentPosition. It works in the cases I tested, but rounding the fallback would keep the column contract integer-valued.
| } | ||
|
|
||
| var rowCount = 10; | ||
| var rowCount = 5.5; |
There was a problem hiding this comment.
rowCount = 5.5 looks like a leftover from testing fractional row heights; intentional?
| } | ||
|
|
||
| /** | ||
| * Calculates the width of the text up to a specific scrrenColumn on a given screen row. |
There was a problem hiding this comment.
nit: typo, scrrenColumn (also ends up in the generated types/ace-modules.d.ts)
| var m0 = 0.7, m1 = 0.1, m2 = 0.3, m3 = 0.82; | ||
| var t1 = 100, t2 = 20; | ||
| function testTransform() { | ||
| fontMetrics.config.$transformData = null; //FIXME |
| * @related EditSession.documentToScreenPosition | ||
| **/ | ||
| screenToDocumentPosition(screenRow: number, screenColumn: number, offsetX?: number): Point; | ||
| screenToDocumentPosition(screenRow: number, screenColumn: number): Point; |
There was a problem hiding this comment.
This is a public API break: screenToDocumentPosition loses its documented offsetX param, and BidiHandler loses getPosLeft/getSelections/offsetToCol/updateCharacterWidths. Embedders calling these will break at runtime, not just in types. Probably fine given the old bidi pixel model is gone, but worth a changelog note and a decision on whether this ships as a minor or major bump.
Open kitchen-sink @ c7be8dd97af41183600195164d17e00f387c23f6
Open kitchen-sink @ 90b4eee66710148337aee06de5ad6e46df0b347f
Open kitchen-sink @ 5a08e666570cde601c49a89c3cf7ee66d32ba8de
Open kitchen-sink @ bed04381bebaa0464636dbed3dbd1c95e6edc99e
Open kitchen-sink @ 2de603755db99236770c3d1d096aac6583f4e8bf
Open kitchen-sink @ 6138c2d3378fc9b489a279145fc98be977233005
Open kitchen-sink @ d7b2cdd7cb12149184fd7f0f0475cc2f846773c1
Open kitchen-sink @ a99a4870bb7d2ec8e7d16cba766645c75e0d713c
Open kitchen-sink @ 60ce0b14fe5032b4fd5789871fa578a652d72fa5
Open kitchen-sink @ 33d1040444586512d70f0d4c28302e94879cf2da
Open kitchen-sink @ 0e9e8bec27cd3677fc50a77343426c4e7bd09a39
Open kitchen-sink @ fca477cbaef5a4f1917382eeed2fd300ac928c60
Open kitchen-sink @ c6c9f87a587d99fb015adf3c905339d16bc54ee0
Open kitchen-sink @ 32ae297abead042c6680563f46b8e10dcb6483ec
Open kitchen-sink @ 40e5bdc846720749cb5392b96a1181744b87c3bb
Open kitchen-sink @ 8f9018cdcadd14972e6cd8d2c6aa0c6a1ba25aeb